[native] Add Arrow Flight Connector by Rijin-N · Pull Request #24082 · prestodb/presto

Rijin-N · 2024-11-19T12:06:43Z

Description

Add Prestissimo support for Arrow Flight connectors.

The native Arrow Flight connector can be used to connect to any Arrow Flight enabled Data Source. The metadata layer is handled by the Presto coordinator and does not need to be re-implemented in C++. Any Java connector that inherits from presto-base-arrow-flight can use this connector as it's counterpart for the Prestissimo layer.

Different Arrow-Flight enabled data sources can differ in authentication styles. A plugin-style interface is provided to handle such cases with custom authentication code.

Motivation and Context

RFC: https://github.com/prestodb/rfcs/blob/main/RFC-0004-arrow-flight-connector.md#prestissimo-implementation

Impact

Arrow Flight based connectors can be used with Prestissimo

Test Plan

Unit Tests set up a mock arrow flight server and exchange data using the new connector.

Contributor checklist

Please make sure your submission complies with our development, formatting, commit message, and attribution guidelines.
PR description addresses the issue accurately and concisely. If the change is non-trivial, ~~a GitHub Issue~~ an RFC is referenced.
Documented new properties (with its default value), SQL syntax, functions, or other functionality.
If release notes are required, they follow the release notes guidelines.
Adequate tests were added if applicable.
CI passed.

Release Notes

Please follow release notes guidelines and fill in the release notes below.

== RELEASE NOTES ==

Prestissimo (Native Execution)
* Add support for Arrow Flight connectors :pr:`24082`

linux-foundation-easycla · 2024-11-19T12:06:47Z

The committers listed above are authorized under a signed CLA.

✅ login: Rijin-N (bd02070, 34d15fe, 1624436, 5c6eaf7, 5715883, b4f5cf0)
✅ login: ashkrisk / name: Ashwin Krishna Kumar (4e7433f, 294126b, 3c8ac31)

Rijin-N · 2024-11-19T12:31:11Z

This PR requires a velox support PR facebookincubator/velox#11588, to install Arrow Flight dependencies.

prestodb-ci · 2024-11-20T23:17:39Z

Saved that user @Rijin-N is from IBM

majetideepak · 2024-11-22T19:15:50Z

presto-native-execution/Makefile

 CMAKE_FLAGS += -DENABLE_ALL_WARNINGS=${ENABLE_WALL}
 CMAKE_FLAGS += -DCMAKE_PREFIX_PATH=$(CMAKE_PREFIX_PATH)
 CMAKE_FLAGS += -DCMAKE_BUILD_TYPE=$(BUILD_TYPE)
+CMAKE_FLAGS += -DPRESTO_ENABLE_ARROW_FLIGHT_CONNECTOR=$(PRESTO_ENABLE_ARROW_FLIGHT_CONNECTOR)


We should stop adding these and instead use EXTRA_CMAKE_FLAGS externally when invoking make

presto-native-execution/presto_cpp/main/connectors/arrow_flight/ArrowFlightConnector.cpp

BryanCutler · 2024-11-22T23:11:16Z

presto-native-execution/presto_cpp/main/connectors/arrow_flight/ArrowFlightConnector.cpp

+
+  CallOptionsAddHeaders callOptsAddHeaders{};
+  FlightCallOptions& callOpts = callOptsAddHeaders;
+  AddCallHeaders& headerWriter = callOptsAddHeaders;


You can just add headers directly to the FlightCallOptions, you don't need to use the AddCallHeaders interface

The idea here was to have an append-only interface for adding headers to the FlightCallOptions.

Without this, the Authenticator interface would need to accept a mutable reference to FlightCallOptions instead, which theoretically allows the authenticator to modify other parts of the FlightCallOptions, which is not desirable.

It's also less confusing when creating Authenticator implementations, since you don't need to worry about which parts of the FlightCallOptions should or should not be changed.

presto-native-execution/presto_cpp/main/connectors/arrow_flight/ArrowFlightConnector.cpp

aditi-pandit

Some common themes
i) Please add a copyright header to all the new files added in this PR.
ii) Please follow the test case naming as per conventions in https://github.com/facebookincubator/velox/blob/main/CODING_STYLE.md#naming-conventions

aditi-pandit · 2024-11-26T08:47:55Z

presto-native-execution/presto_cpp/main/CMakeLists.txt

 set_property(TARGET presto_server_lib PROPERTY JOB_POOL_LINK
                                               presto_link_job_pool)

+if(PRESTO_ENABLE_ARROW_FLIGHT_CONNECTOR)


These lines should be in presto_cpp/main/connectors/arrow_flight/CMakeLists.txt

aditi-pandit · 2024-11-26T08:56:07Z

presto-native-execution/presto_cpp/main/PrestoServer.cpp

      std::make_unique<IcebergPrestoToVeloxConnector>("iceberg"));
  registerPrestoToVeloxConnector(
      std::make_unique<TpchPrestoToVeloxConnector>("tpch"));
+#ifdef PRESTO_ENABLE_ARROW_FLIGHT_CONNECTOR


The previous entries are for the base Velox connectors that we will mostly add by default.

Like in other parts of this code, we should add this registration in ArrowFlightConnector.cpp/.hpp if possible.

Another option is to add a ConnectorRegistration.h(cpp) file in presto_cpp/main/connectors folder that includes all the registrations of connectors in its sub-directories and only call that function from here.

e.g say for Windowfunctions in prestosql there is https://github.com/facebookincubator/velox/blob/main/velox/functions/prestosql/window/WindowFunctionsRegistration.h that has all individual function registrations.

aditi-pandit · 2024-11-26T09:26:22Z

presto-native-execution/presto_cpp/main/types/PrestoToVeloxConnector.cpp

  return std::make_unique<protocol::tpch::TpchConnectorProtocol>();
 }
+
+#ifdef PRESTO_ENABLE_ARROW_FLIGHT_CONNECTOR


This logic should be moved to a file in presto_cpp/main/arrow_flight (ArrowPrestoToVeloxConnector.h/.cpp). The registration logic should be part of ConnectorRegistration file proposed.

aditi-pandit · 2024-11-26T09:27:49Z

presto-native-execution/presto_cpp/main/TaskManager.cpp

    for (const auto& protocolSplit : source.splits) {
      auto split = toVeloxSplit(protocolSplit);
      if (split.hasConnectorSplit()) {
+        // Since the extra credential data is coming from TaskUpdateRequest


There shouldn't be such special case code in TaskManager.cpp.

The more appropriate way to do this is to add the extraCredentials as a parameter in the toVeloxSplit logic and move this specific logic over to the ArrowToPrestoVeloxConnector::toVeloxSplit method.

presto-native-execution/presto_cpp/main/connectors/CMakeLists.txt

aditi-pandit · 2024-11-26T13:46:30Z

...to-native-execution/presto_cpp/main/connectors/arrow_flight/tests/StaticFlightServerTest.cpp

+
+std::unique_ptr<StaticFlightServer> StaticFlightServerTest::server;
+
+TEST_F(StaticFlightServerTest, ServerTest) {


Test names start with lower case. Maybe you can call this test "basic" inline with other tests in the code-base.

aditi-pandit · 2024-11-26T13:47:13Z

...to-native-execution/presto_cpp/main/connectors/arrow_flight/tests/StaticFlightServerTest.cpp

+std::unique_ptr<StaticFlightServer> StaticFlightServerTest::server;
+
+TEST_F(StaticFlightServerTest, ServerTest) {
+  auto sampleTable = makeArrowTable(


Its important to add other types as well... Would be great to demo all the scalar and complex types that are supported by this API.

The StaticFlightServer is just a thin wrapper around the basic Flight Server provided by the Arrow Flight SDK. Testing this server against specific datatypes is something better reserved for the unit tests in the Arrow C++ SDK. The test here is just to make sure that our wrapper is working and we're able to perform simple operations.

Note that the StaticFlightServer isn't actually used in the Flight connector code - it is only used in other unit tests for validating the Flight connector against a "real" server.

That said, we do test for all the different types supported by the Flight connector in ArrowFlightConnectorDataTypeTest.cpp. These test the full flow of data from the StaticFlightServer to the connector and out. We're testing with ints, reals, string types, date and time types etc.

aditi-pandit · 2024-11-26T13:49:33Z

...tive-execution/presto_cpp/main/connectors/arrow_flight/tests/ArrowFlightConnectorTlsTest.cpp

+                {FlightConfig::kServerVerify, "true"}})) {}
+};
+
+TEST_F(FlightConnectorTlsNoCertTest, TlsNoCertTest) {


Change test names to start with lowercase letters.

aditi-pandit · 2024-11-26T13:52:59Z

...tive-execution/presto_cpp/main/connectors/arrow_flight/tests/ArrowFlightConnectorTlsTest.cpp

+  FlightConnectorTlsNoCertTest()
+      : FlightConnectorTlsTestBase(std::make_shared<velox::config::ConfigBase>(
+            std::unordered_map<std::string, std::string>{
+                {FlightConfig::kServerVerify, "true"}})) {}


Seems like this is always a bad config ? Can we error this at the outset itself ?

This isn't necessarily a bad config. We could be dealing with servers which use certificates signed by trusted CAs, making additional certificates unnecessary.

In our case, this is a purely negative test because our test server is set up with a self-signed certificate, and it's impractical to acquire a certificate signed by a trusted CA just for this purpose.

aditi-pandit · 2024-11-26T14:20:43Z

...execution/presto_cpp/main/connectors/arrow_flight/tests/ArrowFlightConnectorDataTypeTest.cpp

+      .assertResults(makeRowVector({decimalVecBigInt}));
+}
+
+TEST_F(FlightConnectorDataTypeTest, GeneralTest) {


Rename "allTypes"

The native Arrow Flight connector can be used to connect to any Arrow Flight enabled Data Source. The metadata layer is handled by the Presto coordinator and does not need to be re-implemented in C++. Any Java connector that inherits from `presto-base-arrow-flight` can use this connector as it's counterpart for the Prestissimo layer. Different Arrow-Flight enabled data sources can differ in authentication styles. A plugin-style interface is provided to handle such cases with custom authentication code. RFC: https://github.com/prestodb/rfcs/blob/main/RFC-0004-arrow-flight-connector.md#prestissimo-implementation Co-authored-by: Ashwin Kumar <Ashwin.Kumar6@ibm.com> Co-authored-by: Rijin-N <rijin.n@ibm.com> Co-authored-by: Nischay Yadav <Nischay.Yadav@ibm.com>

czentgr · 2024-12-02T21:57:59Z

presto-native-execution/presto_cpp/main/connectors/arrow_flight/ArrowFlightConnector.cpp

+}
+
+RowVectorPtr FlightDataSource::projectOutputColumns(
+    std::shared_ptr<arrow::RecordBatch> input) {


This can be const&.

czentgr · 2024-12-02T21:59:42Z

presto-native-execution/presto_cpp/main/connectors/arrow_flight/ArrowFlightConnector.cpp

+using namespace velox;
+using namespace velox::connector;
+
+// wrapper for CallOptions which doesn't add any members variables


Nit: when leaving comments make sure to fllow the comment guidelines

must be complete sentence (period at end and usually capitalization)

etc
This occurs multiple times in a few files.

czentgr · 2024-12-02T22:07:18Z

presto-native-execution/presto_cpp/main/connectors/arrow_flight/ArrowFlightConnector.h

+  uint64_t completedRows_ = 0;
+  uint64_t completedBytes_ = 0;
+  std::shared_ptr<auth::Authenticator> authenticator_;
+  velox::memory::MemoryPool* pool_;


Nit: can be velox::memory::MemoryPool* const pool_;

czentgr · 2024-12-02T22:14:22Z

presto-native-execution/presto_cpp/main/connectors/arrow_flight/ArrowFlightConnector.h

+  explicit ArrowFlightConnectorFactory(const char* name)
+      : ConnectorFactory{name} {}
+
+  ArrowFlightConnectorFactory(const char* name, const char* authenticatorName)


Why not use std::string_view instead of const char* for the authenticatorName?

Why do we need this constructor in the first place? It doesn't seem to be used at all.
The authenticator comes from the FlightConfig. Assuming there is a different Authenticator implementation why would it not be named/configured in the FlightConfig and instead be hard coded at registration time?

Why do we need this constructor in the first place?

The idea here is to be able to register different connector names which use the same Flight connector code, but use different Authenticators. This would avoid the need to have two separate config properties to fully qualify the constructor; instead of specifying connector.name and arrow-flight.authenticator.name, you simply specify connector.name and are good to go.

This would tie in a little better with the Java version, where authentication needs to be handled by subclasses of presto-base-arrow-flight, each of which are independent connectors.

Also note that the config arrow-flight.authenticator.name is not present in the base Java Flight connector. A concrete connector extension would need to introduce this config explicitly to be able to switch between between C++ Authenticators within the same connector.
https://github.com/prestodb/presto/pull/23032/files#diff-9b8ea7947728279eacfeb69fc0492cb2390c3dd65897bc9148ecf1d021d37332

czentgr · 2024-12-02T23:15:18Z

presto-native-execution/presto_cpp/main/connectors/arrow_flight/CMakeLists.txt

+target_link_libraries(
+  presto_flight_connector
+  velox_connector
+  arrow


What is arrow referring to here? It doesn't look like a target but something that cam from PkgConfig? How does this differ to ${ArrowFlight_LIBRARIES}?

Done. Removed arrow, Actually ${ArrowFlight_LIBRARIES} are enough here.

czentgr · 2024-12-02T23:45:15Z

presto-native-execution/presto_cpp/main/connectors/arrow_flight/auth/Authenticator.h

+
+#define AFC_REGISTER_AUTH_FACTORY(factory)                                     \
+  namespace {                                                                  \
+  static bool FB_ANONYMOUS_VARIABLE(g_ConnectorFactory) = ::facebook::presto:: \


I'm not understanding the reason for keeping this static bool around - to indicate a successful registration of NoOpAuthenticatorFactory. Once the factory is registered it is in the list. What's the point of having the static bool of the fact that it was registered stored?
The check that the registration was successful was already done (via VELOX_CHECK). Nothing is checking this bool again.

This variable is not meant to be used anywhere, which is why it's marked as an FB_ANONYMOUS_VARIABLE. This only reason this variable exists is to ensure that it is statically initialized at runtime, and thus ensure that the Authenticator in question is statically registered.

If C++ had static blocks similar to Java, we could have used that instead.

czentgr · 2024-12-02T23:47:27Z

presto-native-execution/presto_cpp/main/connectors/arrow_flight/ArrowFlightConnector.h

+      : Connector{id},
+        flightConfig_{std::make_shared<FlightConfig>(config)},
+        authenticator_{
+            // auth::getAuthenticatorFactory(config_->authenticatorName())


Is this needed?

czentgr · 2024-12-02T23:57:37Z

...execution/presto_cpp/main/connectors/arrow_flight/tests/ArrowFlightConnectorDataTypeTest.cpp

+      .assertResults(makeRowVector({boolVec}));
+}
+
+TEST_F(FlightConnectorDataTypeTest, IntType) {


better name: integerTypes.
Please see the other comment on naming for the tests (lowerCase) and maybe fix some of the names along the way.

czentgr · 2024-12-03T00:09:43Z

...tive-execution/presto_cpp/main/connectors/arrow_flight/tests/utils/FlightConnectorTestBase.h

+ protected:
+  explicit FlightConnectorTestBase(
+      std::shared_ptr<velox::config::ConfigBase> config)
+      : config_{config} {}


std::move(config)

czentgr · 2024-12-03T03:02:32Z

presto-native-execution/scripts/setup-adapters.sh

+  install_abseil
+  install_grpc
+
+  wget_and_untar https://archive.apache.org/dist/arrow/arrow-${ARROW_VERSION}/apache-arrow-${ARROW_VERSION}.tar.gz arrow


Lets use the source from github https://github.com/apache/arrow/archive/apache-arrow-15.0.0.tar.gz. apache.org can have issues with downloads.

steveburnett

I do not find any documentation for me to review.

Should "Arrow Flight connector" be added to the Supported Use Cases topic in the Presto C++ documentation?

Rijin-N · 2024-12-05T10:11:13Z

Hi @majetideepak @BryanCutler @aditi-pandit @czentgr , We have addressed or responded with our reasoning to all the comments you provided. Could you please review and let us know if there is anything else that needs to be done. Thanks.

Note: This format checker is working fine locally for us. We're not sure why it is failing in this PR.

steveburnett

Thank you for the doc! Some suggestions for the data/README.md text.

presto-native-execution/presto_cpp/main/connectors/arrow_flight/tests/data/README.md

czentgr

Please rebase too.

czentgr · 2025-01-03T19:32:28Z

presto-native-execution/presto_cpp/main/connectors/arrow_flight/ArrowFlightConnector.cpp

+
+std::optional<RowVectorPtr> FlightDataSource::next(
+    uint64_t size,
+    velox::ContinueFuture& future) {


Here and in the declaration comment out the unused variable name with new name unused to only leave the type signature.
/* unused */
Doesn't seem like it is used so lets indicate it.

czentgr · 2025-01-03T20:25:21Z

presto-native-execution/presto_cpp/main/connectors/arrow_flight/ArrowFlightConnector.h

+  explicit ArrowFlightConnectorFactory(
+      const char* name,
+      const char* authenticatorName = nullptr)
+      : ConnectorFactory{name}, authenticatorName_{authenticatorName} {}


Nit: Lets use () instead of {} for the initializer list for consistency.

ethanyzhang · 2025-01-08T19:18:03Z

Hi @Rijin-N @ashkrisk based on the discussion we had with Shweta and Sandhya, we are gonna take over this work, @BryanCutler will help move this PR over the finishing line. We keep everyone who worked on this PR as co-authors. Thank you. cc @tdcmeehan

Rijin-N · 2025-01-09T11:03:29Z

Hi @ethanyzhang, We have addressed all the comments so far. You can take over from here.
cc: @ashkrisk

steveburnett

LGTM! (docs)

Used "View file" in GitHub to view the .md files as they will be published. Looks good, thank you!

BryanCutler · 2025-02-06T18:29:26Z

Superseded by #24504

Rijin-N requested a review from a team as a code owner November 19, 2024 12:06

Rijin-N mentioned this pull request Nov 19, 2024

Add support for Arrow Flight package installation facebookincubator/velox#11588

Closed

majetideepak requested review from aditi-pandit, czentgr and majetideepak November 19, 2024 12:43

ethanyzhang added the from:IBM PR from IBM label Nov 20, 2024

prestodb-ci requested a review from a team November 20, 2024 23:17

majetideepak reviewed Nov 22, 2024

View reviewed changes

BryanCutler reviewed Nov 22, 2024

View reviewed changes

aditi-pandit reviewed Nov 26, 2024

View reviewed changes

Rijin-N force-pushed the arrow-flight-connector branch 2 times, most recently from e51be47 to 9f48cec Compare November 27, 2024 08:40

Rijin-N requested review from a team and steveburnett as code owners November 27, 2024 08:40

Rijin-N requested a review from presto-oss November 27, 2024 08:40

Rijin-N force-pushed the arrow-flight-connector branch from 9f48cec to bd02070 Compare November 27, 2024 11:57

ashkrisk and others added 3 commits November 29, 2024 11:35

Review comment fixes

294126b

Review comment fixes (code restructuring)

34d15fe

Minor correction in code restructuring

1624436

czentgr reviewed Dec 3, 2024

View reviewed changes

Rijin-N and others added 3 commits December 3, 2024 03:36

Review comment fixes (unit test related fixes)

5715883

Move init of connection properties to constructor

4e7433f

Review comment fixes (renamed tests and corrected comments)

5c6eaf7

steveburnett reviewed Dec 4, 2024

View reviewed changes

Remove no-op

3c8ac31

steveburnett requested changes Jan 3, 2025

View reviewed changes

czentgr reviewed Jan 4, 2025

View reviewed changes

Doc related review comment update

b4f5cf0

steveburnett approved these changes Jan 9, 2025

View reviewed changes

BryanCutler mentioned this pull request Feb 5, 2025

[native] Add Apache Arrow Flight Connector #24504

Merged

6 tasks

ethanyzhang closed this Mar 20, 2025


		std::unique_ptr<StaticFlightServer> StaticFlightServerTest::server;

		TEST_F(StaticFlightServerTest, ServerTest) {

Conversation

Rijin-N commented Nov 19, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Motivation and Context

Impact

Test Plan

Contributor checklist

Release Notes

Uh oh!

linux-foundation-easycla bot commented Nov 19, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Rijin-N commented Nov 19, 2024

Uh oh!

prestodb-ci commented Nov 20, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

aditi-pandit left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ashkrisk Nov 28, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Rijin-N commented Nov 19, 2024 •

edited

Loading

linux-foundation-easycla bot commented Nov 19, 2024 •

edited

Loading

ashkrisk Nov 28, 2024 •

edited

Loading

ashkrisk Dec 3, 2024 •

edited

Loading